Update submodule for txservice#157
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughUpdated the Changes
Sequence Diagram(s)sequenceDiagram
participant HA as ha_eloq.cc
participant BR as BatchReadTxRequest
participant TM as TxManager
participant Store as Storage Engine
HA->>BR: construct(request with Txm, true, ...)
BR->>TM: pass Txm + flags
TM->>Store: execute batch read
Store-->>TM: results
TM-->>BR: results
BR-->>HA: return batch results
note right of BR `#DFF2E1`: New boolean argument provided to influence\nrequest behavior/validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
xiexiaoy
force-pushed
the
point_read_on_miss
branch
2 times, most recently
from
8664579 to
f99ed99
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
storage/eloq/ha_eloq.cc (1)
6836-6837: SkIndexScanNext: enabling point-read-on-miss for PK batch loads.Assuming the new bool is “point_read_on_miss”, this should reduce false negatives when SK→PK post-filtering. Be aware it can increase I/O on large range scans; keep as-is, but consider toggling by a session/global knob if this path becomes hot.
Same signature check as above applies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
storage/eloq/ha_eloq.cc(4 hunks)storage/eloq/tx_service(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- storage/eloq/tx_service
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.624Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2770-2781
Timestamp: 2025-11-07T05:12:43.190Z
Learning: In eloqdata/eloqsql, storage/eloq/ha_eloq.cc (KV_ELOQDS path): for DATA_STORE_TYPE_ELOQDSS_ROCKSDB (local RocksDB), each eloqsql instance owns its own RocksDB instance, and passing create_if_missing=true to DataStoreService::StartService is intentional and safe.
📚 Learning: 2025-11-07T07:10:40.624Z
Learnt from: githubzilla
Repo: eloqdata/eloqsql PR: 154
File: storage/eloq/ha_eloq.cc:2571-2623
Timestamp: 2025-11-07T07:10:40.624Z
Learning: Repo: eloqdata/eloqsql PR: 154
File/Area: storage/eloq/ha_eloq.cc (eloq_init_func cluster membership resolution)
Learning: In this repo’s ng_config, the local node is guaranteed to be a candidate (NodeConfig::is_candidate_ == true). Therefore, assigning native_ng_id only when is_candidate_ is true is acceptable; the key improvement is to break early from both loops once the local node is found.
Applied to files:
storage/eloq/ha_eloq.cc
🔇 Additional comments (3)
storage/eloq/ha_eloq.cc (3)
9705-9705: Minor formatting tweak.Trailing newline at
maria_declare_plugin_endis fine. LGTM.
9428-9432: Changes verified as consistent with existing codebase patterns.The two
BatchReadTxRequestconstructor calls in the codebase (lines 6833 and 9428) are identical in parameter order and count. Both calls maintainfor_updateandfor_sharein positions 4 and 5, and both consistently usepoint_read_on_miss=trueas the final argument. No old-signature calls with different parameter counts exist.
5642-5643: Verify parameter consistency across all BatchReadTxRequest call sites.Unable to locate the BatchReadTxRequest definition in the repository (tx_service module appears incomplete in this checkout). However, I found a potential issue: the call at lines 5640–5643 passes 11 arguments, while similar calls at lines 6833–6836 and 9428–9431 pass only 10 arguments. The extra
0between the penultimatetrueand finaltruemay indicate an incomplete refactoring.Please confirm:
- All three call sites match the current constructor signature in tx_service/include/tx_request.h
- The 11-arg variant at line 5640 is intentional (possibly adding a point-read flag or timeout)
- No other call sites need updating
xiexiaoy
force-pushed
the
point_read_on_miss
branch
from
f99ed99 to
c047a1a
Compare
2 participants
eloqdata/tx_service#195
Summary by CodeRabbit
Bug Fixes
Chores